Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update test_resource_manager.cpp - Fixed Typo #1609

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

Parker-Drouillard
Copy link
Contributor

Fixed typo causing errors during build, test case was written as initilizable and not initializable as was expected by the tests.

Fixed typo causing errors during build, test case was written as initilizable and not initializable as was expected by the tests.
@Parker-Drouillard
Copy link
Contributor Author

Apologies, I also missed correcting the typo on line 118 of the same file, and on line 258 in ros2_control/install/ros2_control_test_assets/include/ros2_control_test_assets/descriptions.hpp at line 458.

Not quite used to using the web editor for pull requests. I am uncertain the appropriate way to handle this?

@christophfroehlich
Copy link
Contributor

Fixed typo causing errors during build, test case was written as initilizable and not initializable as was expected by the tests.

You are right about a typo, but it is still not correct: it should be uninitializable 😀, if that's even an English word (according to deepl it is).
What errors did you get by compiling the master? The scoped trace macro has an arbitrary string, and the test name is not used elsewhere?

Not quite used to using the web editor for pull requests. I am uncertain the appropriate way to handle this?

You can checkout the PR locally, or just open the patch-1 branch of your fork in the github UI, edit file, and commit directly to the branch.

@Parker-Drouillard
Copy link
Contributor Author

Fixed typo causing errors during build, test case was written as initilizable and not initializable as was expected by the tests.

You are right about a typo, but it is still not correct: it should be uninitializable 😀, if that's even an English word (according to deepl it is). What errors did you get by compiling the master? The scoped trace macro has an arbitrary string, and the test name is not used elsewhere?

Not quite used to using the web editor for pull requests. I am uncertain the appropriate way to handle this?

You can checkout the PR locally, or just open the patch-1 branch of your fork in the github UI, edit file, and commit directly to the branch.

While using colcon build, I got a blocking error stating the following, among other unrelated errors (I replaced my local path with ~~)

~~/src/ros2_control/controller_manager/test/test_controller_manager_urdf_passing.cpp: In member function ‘virtual void TestControllerManagerWithTestableCM_expect_to_failure_when_invalid_urdf_is_given_and_be_able_to_submit_new_robot_description_Test::TestBody()’:
~~/src/ros2_control/controller_manager/test/test_controller_manager_urdf_passing.cpp:86:40: error: ‘minimal_uninitializable_robot_urdf’ is not a member of ‘ros2_control_test_assets’; did you mean ‘minimal_unitilizable_robot_urdf’?
   86 |   msg.data = ros2_control_test_assets::minimal_uninitializable_robot_urdf;
      |                                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                                        minimal_unitilizable_robot_urdf

@Parker-Drouillard
Copy link
Contributor Author

also I am laughing at the fact I didn't catch the fact that it's in and not un. Good catch on your part

Fixed ininitializable to uninitializable (Even for a native English speaker that's a rough one)
@bmagyar
Copy link
Member

bmagyar commented Jul 12, 2024

The changes look legit but we fixed those typos months ago and had many releases since. I'm wondering how a few creeped back into the codebase especially w the CI being happy

@christophfroehlich
Copy link
Contributor

I'm wondering the same, maybe @Parker-Drouillard has a very old release installed as binary, and colcon uses now the test assets from the old release instead of the new one from the src folder?

@Parker-Drouillard
Copy link
Contributor Author

I'm wondering the same, maybe @Parker-Drouillard has a very old release installed as binary, and colcon uses now the test assets from the old release instead of the new one from the src folder?

I doubt that, I only started with ROS a week ago, and the hardware I'm on was a fresh install as of two days ago.

I have seen the typo in both the master and humble branches. This is my first ever open source contribution though so it's possible I'm doing something wrong!

@Parker-Drouillard
Copy link
Contributor Author

From what I remember earlier, the changes had been pushed back in something like 5 days ago by someone.

Copy link

codecov bot commented Jul 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.30%. Comparing base (465b58f) to head (98f807b).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1609   +/-   ##
=======================================
  Coverage   87.30%   87.30%           
=======================================
  Files         108      108           
  Lines        9866     9866           
  Branches      890      890           
=======================================
  Hits         8614     8614           
  Misses        929      929           
  Partials      323      323           
Flag Coverage Δ
unittests 87.30% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...e_interface_testing/test/test_resource_manager.cpp 99.32% <100.00%> (ø)

@christophfroehlich
Copy link
Contributor

The typo fix of the current commit is fine, but should not change anything regarding your build failure. And our CI on the master as well as humble branch is happy..

@saikishor
Copy link
Member

I'm wondering the same, maybe @Parker-Drouillard has a very old release installed as binary, and colcon uses now the test assets from the old release instead of the new one from the src folder?

I believe yes. I faced the same as your said @christophfroehlich

@Parker-Drouillard
Copy link
Contributor Author

I'm wondering the same, maybe @Parker-Drouillard has a very old release installed as binary, and colcon uses now the test assets from the old release instead of the new one from the src folder?

I believe yes. I faced the same as your said @christophfroehlich

Is this possible considering I'm on a fresh install of Ubuntu 22.04 as of a few days ago? I also only started getting into ros2 about a week ago, am I somehow pulling old binaries?

@christophfroehlich
Copy link
Contributor

Yes, because since a couple of months there is no rolling release to 22.04 any more -> there was a switch to noble 24.04. I'm not sure what we can do to avoid such a confusion. @saikishor a compile-time error if the wrong version of hardware_interface_testing is to be built?

@saikishor
Copy link
Member

Yes, because since a couple of months there is no rolling release to 22.04 any more -> there was a switch to noble 24.04. I'm not sure what we can do to avoid such a confusion. @saikishor a compile-time error if the wrong version of hardware_interface_testing is to be built?

I've tried using jammy and compiling it from source and it works. I have been working on Jammy for last 2 months

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bmagyar bmagyar merged commit 1f8ca08 into ros-controls:master Jul 16, 2024
19 checks passed
@Parker-Drouillard Parker-Drouillard deleted the patch-1 branch July 17, 2024 14:51
christophfroehlich added a commit that referenced this pull request Aug 12, 2024
* [ResourceManager] Make destructor virtual for use in derived classes (#1607)

* Fix typos in test_resource_manager.cpp (#1609)

* [CM] Remove support for the description parameter and use only `robot_description` topic (#1358)

---------

Co-authored-by: Felix Exner <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>

* move critical variables to the private context (#1623)

* Fix controller starting with later load of robot description test (#1624)

* Fix the duplicated restart of the controller_manager services initialization

* Scope the ControllerManagerRunner to avoid malloc and other test issues

* reorder the tests for consistent log at the end

* Remove noqa (#1626)

* Unused header cleanup (#1627)

* Create debugging.rst (#1625)


---------

Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>

* Update changelogs

* 4.14.0

* add missing rclcpp logging include for Humble compatibility build (#1635)

* [CM] Remove deprecated spawner args (#1639)

* Add a pytest launch file to test ros2_control_node (#1636)

* Fix rst markup (#1642)

* Fix rqt_cm paragraph

* Fix indent

* CM: Add missing includes (#1641)

* [RM] Add `get_hardware_info` method to the Hardware Components (#1643)

* Fix the namespaced controller_manager spawner + added tests (#1640)

* Bump version of pre-commit hooks (#1649)

Co-authored-by: christophfroehlich <[email protected]>

* Add missing include for executors (#1653)

* Update changelogs

* 4.15.0

* refactor SwitchParams to fix the incosistencies in the spawner tests (#1638)

* Modify test with missing CM to have a timeout

* Catch exception when CM services are not found

And print the error and exit in the application

* Exit with code 1 on unreached CM

---------

Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Parker Drouillard <[email protected]>
Co-authored-by: Dr. Denis <[email protected]>
Co-authored-by: Christoph Froehlich <[email protected]>
Co-authored-by: Christoph Fröhlich <[email protected]>
Co-authored-by: Henry Moore <[email protected]>
Co-authored-by: Lennart Nachtigall <[email protected]>
Co-authored-by: Sai Kishor Kothakota <[email protected]>
Co-authored-by: Bence Magyar <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: christophfroehlich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants